Skip to content

Conversation

@not-matthias
Copy link
Member

No description provided.

@not-matthias not-matthias force-pushed the cod-1710-instrument-hooks-support-new-analysis-mode branch 4 times, most recently from 5737ac4 to c4f95c0 Compare November 21, 2025 15:18
@not-matthias not-matthias requested a review from Copilot November 21, 2025 15:19
Copilot finished reviewing on behalf of not-matthias November 21, 2025 15:22
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds support for a new "analysis" integration mode, allowing the instrumentation hooks to operate in three distinct modes: Valgrind, Perf, and Analysis. The implementation refactors existing code to use a common helper for FIFO-based instruments.

  • Introduces the IntegrationMode enum and related protocol commands for mode detection
  • Creates a generic FifoInstrument helper that validates the integration mode during initialization
  • Refactors PerfInstrument to use the new helper and moves related code to runner_fifo.zig

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/shared.zig Adds IntegrationMode enum and new Command variants for integration mode queries
src/runner_fifo.zig New file containing refactored FIFO communication logic from perf.zig with mode detection
src/instruments/helper.zig New generic FifoInstrument function that creates mode-validated instrument implementations
src/instruments/perf.zig Refactored to use FifoInstrument helper, significantly reduced code
src/instruments/analysis.zig New analysis instrument using the FifoInstrument helper
src/instruments/valgrind.zig Updated init to return error when not instrumented for consistency
src/instruments/root.zig Updated to support analysis mode with proper initialization order and delegation
src/c.zig Removed protocol version validation logic (moved to runner_fifo.zig)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@not-matthias not-matthias marked this pull request as ready for review November 24, 2025 09:51
@not-matthias not-matthias force-pushed the cod-1710-instrument-hooks-support-new-analysis-mode branch from 4ecb2f7 to d673228 Compare November 24, 2025 10:21
@not-matthias not-matthias force-pushed the cod-1710-instrument-hooks-support-new-analysis-mode branch from d673228 to a19e8c3 Compare November 24, 2025 17:04
Copy link
Contributor

@GuillaumeLagrange GuillaumeLagrange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

olgtm

@not-matthias not-matthias force-pushed the cod-1710-instrument-hooks-support-new-analysis-mode branch 2 times, most recently from 3224a77 to a36c4d7 Compare November 27, 2025 11:21
Copy link
Contributor

@GuillaumeLagrange GuillaumeLagrange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

olgtm

@not-matthias not-matthias force-pushed the cod-1710-instrument-hooks-support-new-analysis-mode branch from a36c4d7 to dfad349 Compare November 27, 2025 18:42
@not-matthias not-matthias force-pushed the cod-1710-instrument-hooks-support-new-analysis-mode branch from dfad349 to 075ad83 Compare November 27, 2025 18:47
@not-matthias not-matthias merged commit 075ad83 into main Nov 27, 2025
46 checks passed
@not-matthias not-matthias deleted the cod-1710-instrument-hooks-support-new-analysis-mode branch November 27, 2025 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants